-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
x/ibc: amino json support #8422
Conversation
suite.Require().NoError(err) | ||
expString = fmt.Sprintf( | ||
`{"type":"cosmos-sdk/MsgSubmitMisbehaviour","value":{"client_id":"clientid","client_state":,"signer":"%s"}}`, | ||
// string(sdk.MustSortJSON(types.ModuleCdc.LegacyAmino.MustMarshalJSON(misbehaviour))), // FIXME: header panics on amino json marshal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the misbehaviour headers JSON marshaling is panicking:
panic: Unregistered interface crypto.isPublicKey_Sum [recovered]
panic: Unregistered interface crypto.isPublicKey_Sum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robert-zaremba @clevinson @amaurymartiny @aaronc any hints?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you will need to register that interface here https://github.com/cosmos/cosmos-sdk/blob/master/crypto/codec/amino.go. Not sure how other modules do this since it's not registered already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I think it's actually the Tendermint pubkey interface:
// PublicKey defines the keys available for use with Tendermint Validators
type PublicKey struct {
// Types that are valid to be assigned to Sum:
// *PublicKey_Ed25519
// *PublicKey_Secp256K1
Sum isPublicKey_Sum `protobuf_oneof:"sum"`
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I don't fully understand this code, I can take a look in the morning.
The sdk handles tendermint keys as its own, comes in from abci and gets converted before any usage. This should be done here as well to avoid the dependency on tendermint. I remember doing some work a while ago in the sdk to help with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah so the ibc tendermint header imports some types that use the validator proto msgs (which contain the pubkey). Maybe you can coordinate tomorrow morning with @colin-axner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relaying from this comment from @ebuchman cosmos/gaia#568 (comment), the general consensus seems to be to limit this PR to only include amino support for IBC MsgTransfer
, and no other IBC messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will prune back this pr to just do the change for MsgTransfer
(I might just open a new pr since that might be easiest). How can we test MsgTransfer
with a ledger to ensure that everything does actually work fine? Maybe someone can point me to some tools to use?
Adding the Stargate backport label here, as this PR should be cherry-picked to the |
superseded by #8437 |
Thanks @colin-axner |
closes #8266